Skip to content

Extract SqlBulkCopy column names using dynamic SQL#4092

Merged
paulmedynski merged 7 commits intomainfrom
dev/paul/3714-sqlbulkcopy-sql2016
Apr 7, 2026
Merged

Extract SqlBulkCopy column names using dynamic SQL#4092
paulmedynski merged 7 commits intomainfrom
dev/paul/3714-sqlbulkcopy-sql2016

Conversation

@paulmedynski
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski commented Mar 26, 2026 •
edited
Loading

Description

#3590 allowed SqlBulkCopy to find and work with hidden columns (even if those columns weren't accessible server-side.) It contained a check on the all_columns DMV to make sure that we only checked the graph_type column when querying if that column existed; this was to maintain SQL 2016 compatibility.

#3714 highlighted that both queries are compiled and fail at the point of compilation anyway, so this wasn't accomplishing anything. To make this work, we need to use dynamic SQL to run the column queries. This PR does so.

Supersedes #3719. Original PR by @edwardneal. This PR was created as a direct branch of the SqlClient repo so I can run the new SQL 16 and 17 CI pipeline stages manually to prove this fixes the issue.

Issues

Fixes #3714.

Testing

Manual testing of all test cases against a SQL 2016 instance (via the updated CI pipelines).

@paulmedynski paulmedynski requested a review from a team as a code owner March 26, 2026 13:33
@paulmedynski paulmedynski added this to the 7.1.0-preview1 milestone Mar 26, 2026
@paulmedynski paulmedynski added the Regression 💥 Issues that are regressions introduced from earlier PRs. label Mar 26, 2026
Copilot AI review requested due to automatic review settings March 26, 2026 13:33
@github-project-automation github-project-automation bot moved this to To triage in SqlClient Board Mar 26, 2026
@paulmedynski paulmedynski mentioned this pull request Mar 26, 2026
Closed
@paulmedynski paulmedynski mentioned this pull request Mar 26, 2026
Closed
@paulmedynski paulmedynski moved this from To triage to In review in SqlClient Board Mar 26, 2026
Copilot AI reviewed Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates SqlBulkCopy’s target-column discovery query to use dynamic SQL so that referencing sys.all_columns.graph_type doesn’t cause compilation failures on SQL Server 2016, addressing regression #3714.

Changes:

  • Reworks SqlBulkCopy.CreateInitialQuery() to build and execute the sys.all_columns column-name query via sp_executesql.
  • Adjusts manual test SQL statistics expectations to account for the additional statements executed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopyAllFromReader.cs Updates expected SelectCount / SelectRows due to extra statements from dynamic SQL.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Uses dynamic SQL for column-name extraction to avoid SQL 2016 compilation errors when graph_type is absent.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.51%. Comparing base (60d4b92) to head (8074cd5).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...Client/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs 63.63% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (60d4b92) and HEAD (8074cd5). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (60d4b92) HEAD (8074cd5)
CI-SqlClient 1 0
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4092      +/-   ##
==========================================
- Coverage   73.22%   66.51%   -6.72%     
==========================================
  Files         280      274       -6     
  Lines       43000    65785   +22785     
==========================================
+ Hits        31486    43754   +12268     
- Misses      11514    22031   +10517
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 66.51% <63.63%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings March 26, 2026 17:38
@paulmedynski paulmedynski force-pushed the dev/paul/3714-sqlbulkcopy-sql2016 branch from 5ef395f to 1be29d6 Compare March 26, 2026 17:38
Copilot AI reviewed Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

cheenamalhotra
cheenamalhotra previously approved these changes Mar 26, 2026
View reviewed changes
@paulmedynski
Copy link
Copy Markdown
Contributor Author

paulmedynski commented Mar 27, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 27, 2026

Azure Pipelines successfully started running 2 pipeline(s).

@cheenamalhotra cheenamalhotra added the Hotfix Candidate 🚑 Issues/PRs that are candidate for backporting to earlier supported versions. label Mar 28, 2026
priyankatiwari08
priyankatiwari08 previously approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@priyankatiwari08 priyankatiwari08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, Just added a suggestion to add comment related to explaining why CatalogName is interpolated directly into the dynamic SQL string rather than parameterized

@paulmedynski paulmedynski added Hotfix 7.0.1 When this PR merges, automatically open a PR to cherry-pick to the 7.0.1 branch and removed Hotfix Candidate 🚑 Issues/PRs that are candidate for backporting to earlier supported versions. labels Mar 31, 2026
@paulmedynski
Copy link
Copy Markdown
Contributor Author

paulmedynski commented Apr 1, 2026

Addressed both open review comments in 70269ce:

  1. SQLServerVersion lazy-init fix (Copilot): Changed s_sQLServerVersion field initializer from string.Empty to null (and renamed to s_sqlServerVersion) so the ??= null-coalescing assignment actually triggers. IsAtLeastSQL2017() / IsAtLeastSQL2019() will now correctly query the server version on first access.

  2. CatalogName interpolation comment (@priyankatiwari08): Added an inline comment block explaining that CatalogName and escapedObjectName are interpolated directly because SQL Server doesn't allow parameterizing identifiers, and both values are pre-escaped via SqlServerEscapeHelper.

cheenamalhotra
cheenamalhotra previously approved these changes Apr 2, 2026
View reviewed changes
@edwardneal @paulmedynski
Build column name query using dynamic SQL
49ea581 
This enables the query to compile correctly when the graph_type column is missing
@paulmedynski
Addressed the Copilot comment from the original PR #3719.
8ab9595
@paulmedynski
Added checks to inhibit tests running on SQL 2016 and 2017 that shoul…
b3f052b 
…dn't run there.
@paulmedynski
Address PR review comments
29a3aff 
- Fix SQLServerVersion lazy-init: change field initializer from
  string.Empty to null so the ??= null-coalescing assignment actually
  triggers. This makes IsAtLeastSQL2017/IsAtLeastSQL2019 work correctly.
- Add inline comment explaining why CatalogName and escapedObjectName
  are interpolated directly into the dynamic SQL string (SQL Server does
  not allow parameterizing identifiers).
Copilot AI review requested due to automatic review settings April 2, 2026 11:51
@paulmedynski paulmedynski force-pushed the dev/paul/3714-sqlbulkcopy-sql2016 branch from 70269ce to 29a3aff Compare April 2, 2026 11:51
Copilot AI reviewed Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTestWithTdsServer.cs:97

  • ForceEncryptionRegistryPath no longer handles SQL Server 2016 (major 13). On SQL 2016 it will return string.Empty, and Dispose() later passes that value to RemoveForceEncryptionFromRegistryPath, which may end up opening/setting values on an unintended registry key (or throwing). Add an IsSQL2016() branch that returns the MSSQL13 registry path, and/or defensively avoid calling registry cleanup when the path is empty.
                if (DataTestUtility.IsSQL2022())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL16.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2019())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL15.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2017())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL14.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                return string.Empty;
            }

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTest.cs:59

  • ForceEncryptionRegistryPath no longer has an SQL Server 2016 (major 13) mapping; it falls back to string.Empty. This means the test will skip certificate/registry cleanup in Dispose() on SQL 2016 and can leave machine-level state behind. Add an IsSQL2016() branch returning the MSSQL13 registry path so cleanup works on SQL 2016 as well.
                if (DataTestUtility.IsSQL2022())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL16.{InstanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2019())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL15.{InstanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2017())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL14.{InstanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                return string.Empty;
            }

Copilot AI review requested due to automatic review settings April 2, 2026 17:06
Copilot AI reviewed Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTestWithTdsServer.cs:96

  • ForceEncryptionRegistryPath no longer returns a path for SQL Server 2016 (major 13). On SQL 2016 this will return string.Empty, but Dispose() later calls RemoveForceEncryptionFromRegistryPath(ForceEncryptionRegistryPath) without checking for empty, which can lead to attempting to open/set values on the HKLM root key (or throwing). Add an IsSQL2016() branch mapping to MSSQL13.* or guard against empty before using the path.
                if (DataTestUtility.IsSQL2017())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL14.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                return string.Empty;

@paulmedynski paulmedynski changed the title Fix 3714 | Extract SqlBulkCopy column names using dynamic SQL Extract SqlBulkCopy column names using dynamic SQL Apr 6, 2026
@paulmedynski
Copy link
Copy Markdown
Contributor Author

paulmedynski commented Apr 6, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 6, 2026

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski paulmedynski merged commit 1ec7675 into main Apr 7, 2026
299 checks passed
@paulmedynski paulmedynski deleted the dev/paul/3714-sqlbulkcopy-sql2016 branch April 7, 2026 09:54
@github-project-automation github-project-automation bot moved this from In review to Done in SqlClient Board Apr 7, 2026
github-actions bot pushed a commit that referenced this pull request Apr 7, 2026
@paulmedynski @edwardneal @github-actions
Extract SqlBulkCopy column names using dynamic SQL (#4092)
1b61cbb 
Co-authored-by: Edward Neal <55035479+edwardneal@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Apr 7, 2026
Merged
@AnsonNHS AnsonNHS mentioned this pull request Apr 9, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mdaigle mdaigle mdaigle approved these changes

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@priyankatiwari08 priyankatiwari08 priyankatiwari08 left review comments

+1 more reviewer

@edwardneal edwardneal edwardneal left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Hotfix 7.0.1 When this PR merges, automatically open a PR to cherry-pick to the 7.0.1 branch Regression 💥 Issues that are regressions introduced from earlier PRs.

Projects

SqlClient Board
Status: Done

Milestone

7.1.0-preview1

Development

Successfully merging this pull request may close these issues.

SQL Bulk Copy Fails on SQL Server 2016 / 130

6 participants

@paulmedynski @mdaigle @cheenamalhotra @edwardneal @priyankatiwari08